Repository navigation
fix(web): gate login/refresh commits on a session generation (#310) - #330
Conversation
Treat login, the bootstrap silent refresh, an explicit (401-triggered) refresh, and logout as one browser-session state machine instead of four calls that can race each other. A monotonically increasing generation is bumped by login (a newer session supersedes anything in flight) and by logout (before clearing tokens or awaiting the server call); a refresh's resolution — success or failure — only commits tokens/state while its captured generation is still current, otherwise it is discarded as stale. logout also now cancels any in-flight refresh where possible, and its best-effort server-side revoke failure is logged instead of swallowed silently. Fixes a real security bug: an in-flight refresh completing after logout could resurrect authenticated state, and an older bootstrap refresh could overwrite a newer explicit login.
Nothing secret-shaped belongs in source even in a test file (GitGuardian scans them), and a unique per-run value makes the "did not leak" assertion exact rather than coincidental.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78586b9e5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
| // Superseded while in flight (e.g. a logout landed before this resolved) — | ||
| // do not resurrect a session the user already ended. | ||
| if (sessionGeneration !== generation) throw new StaleSessionError(); |
There was a problem hiding this comment.
When /auth/login is in flight and logout() finishes first—the exact race this check targets—the late successful login response has already applied its Set-Cookie before raw() resolves, so throwing here blocks only the in-memory access token. The browser therefore retains the newly issued HttpOnly refresh token after logout, and a reload can authenticate again through restoreSession(); likewise, a late refresh response can overwrite a newer login's cookie. Because AuthEndpoints.cs sets rotating refresh cookies for both responses, these cookie-mutating requests must be serialized or cancelled rather than gating only their JavaScript completion.
AGENTS.md reference: AGENTS.md:L43-L43
Useful? React with 👍 / 👎.
…he UI Review of #330 found a fifth token-store writer the generation guard never covered. changePassword() committed its response unconditionally, and logout is reachable from every screen via AppLayout's persistent button — so submitting a password change and then logging out before it landed wrote a fresh token back over the cleared session. That is exactly the bug #310 exists to close. Second, StaleSessionError was escaping to callers. currentAccessToken() rethrew it outside apiFetch's try, and both 401-retry branches rethrew it as a "transient" failure. Every screen renders a non-ApiError's .message verbatim, so a user whose refresh merely lost a race to their own login saw "Discarded: superseded by a newer login or an intervening logout." instead of their request simply succeeding. A discarded refresh now resolves against whatever superseded it: a newer login's token is used (the request is valid, so it proceeds), a logout raises a plain 401 without re-firing onUnauthenticated. The marker no longer leaves the module. The existing late-failure test asserted the token store but discarded the parked request's own outcome, so it passed either way; it now asserts the request resolves with its data. Also corrected the logout comment: the abort only reaches a refresh that already holds the cross-tab lock, not one still queued behind another tab. Correctness rests on the generation check, not the abort.
Review of #330 found the generation check only gated JavaScript state. Both /auth/login and /auth/refresh (and change-password) rotate the HttpOnly refresh cookie, and the browser applies Set-Cookie the moment the response arrives — before any of our code runs. So a login landing after logout left a VALID refresh cookie behind: the next reload authenticated straight through restoreSession() and the session the user had ended came back. Discarding the in-memory token never touched that. A discarded cookie-setting response now asks the server to revoke what it issued. Scoped to "the session is actually gone" (no access token): when a newer LOGIN superseded the flight, the cookie belongs to that live session and revoking it would sign the user out of the thing they just did — covered by its own test. The failure branch of refresh is deliberately excluded: a refresh that failed issued no cookie to revoke.
|
Confirmed and fixed — this was the real hole, and my generation guard only covered half the problem. You're right that the check gated JavaScript state while the browser had already applied A discarded cookie-setting response now asks the server to revoke what it issued, so the credential dies with the JS result. One deliberate scoping decision: the revoke only fires when the session is actually gone (no access token). Cancelling or serializing outright would also kill the newer login supersedes older refresh case — but there the cookie belongs to a live session the user just created, and revoking it would sign them out of the thing they just did. That asymmetry has its own test ( Also excluded: the refresh failure branch, which issued no cookie to revoke. Mutation-checked — making the revoke a no-op turns both logout-race tests red on the new assertion. 1212/1212 green, typecheck and build clean. |
CI's per-file gate on src/api/client.ts failed: the cookie-revoke fix added branches that no test reached (statements 90 vs 94, branches 80.7 vs 85). The gate was right — these were real behaviours shipped untested, not a threshold that needed relaxing. Adds the four missing cases, both outcomes on both request paths: a refresh superseded mid-401-retry proceeds on the newer login's token, and one superseded by logout surfaces the original 401 rather than the internal discard marker — for apiFetch and for apiGetBlob, whose retry is a separate branch. Also pins that a request parked on currentAccessToken sees a plain ApiError, since screens render a non-ApiError's message verbatim. client.ts now 96.5 statements / 89.2 branches / 98 lines, comfortably clear of the floors rather than a hair over. The one remaining uncovered span (417-419) predates this branch.
|
CI caught a genuine gap in the cookie-revoke commit — the per-file coverage gate on The gate was right, and the fix was to cover the branches rather than move the floor. What it flagged were real behaviours I'd shipped untested: the supersession handling on the 401-retry path, which is separate from the Four cases added — both outcomes on both request paths:
Plus one pinning that a request parked on
1217/1217 vitest, typecheck and build clean. |
Closes #310.
Problem
Login, bootstrap refresh, explicit refresh, and logout each mutated the token store independently, with no notion of which browser session a completion belonged to. Two real consequences:
Logout also swallowed server-side revoke failures entirely, so a failed revoke was invisible.
Fix
A monotonically increasing session generation in
client.ts, which already owns the token store and the login/logout/refresh entry points.login()bumps the generation before its await — a login is by definition newer than anything in flight — then re-checks before committing tokens.logout()bumps the generation and clears the token synchronously, before any await, and aborts the in-flight refresh (real cancellation on top of the generation backstop). The server revoke is still attempted; a failure is now reported rather than swallowed, without ever logging the token.refreshTokens()captures the generation when the flight starts and re-checks it at settlement — on both success and failure. A mismatch discards the outcome via an internalStaleSessionErrorinstead of committing.The failure side matters as much as the success side: without it, a stale rejection would have torn down a session that had already been legitimately replaced.
On the suggested approach
The orchestrator suggested reusing the
useRef"active target" idiom fromUsersPage.tsx. That idiom guards a React UI target — which dialog a response belongs to — and lives in component state. This bug is a layer below React, in the plain-module session machinery shared by all four entry points, so the equivalent primitive is a module-level counter. The underlying principle is identical (a synchronous guard captured before the await and re-checked before committing); only the container differs.AuthContextneeded no changes — a discarded refresh returnsfalse, which flows through its existing guard.Tests
Nine tests using deferred promises for deterministic races, each paired with a green control so the passing case is pinned too, not just the race:
Mutation-checked: with
client.tsreverted and the tests kept, the five race/observability tests go RED with identical assertion failures; restoring turns them green. Async race tests pass vacuously very easily, so each was confirmed individually rather than as a batch.The token-leak probe uses a runtime-generated value rather than a literal — nothing secret-shaped belongs in source even in a test file, and a unique per-run value makes the "did not leak" assertion exact rather than coincidental.
Verification
npm run typecheck— cleannpx vitest run— 1209/1209 passing, 66 filesnpm run build— succeedsDocs
No GLOSSARY or Help page change. The existing "Session tokens (#145)" glossary entry describes the mechanism (memory-only access token, HttpOnly refresh cookie, cross-tab lock) and this fix closes a timing window within that already-documented design — it adds no concept and makes no existing claim more or less true. The Help page has no session/logout content to correct.
Out of scope
Real-browser coverage of these races belongs to #277, which has no Playwright infrastructure yet; the issue frames that as something #277 gains once this lands, not a deliverable here.